feat(replay): Capture request/response body size for xhr/fetch breadcrumbs #7407

mydea · 2023-03-10T11:09:05Z

This ensures that in Replay we capture more details for xhr/fetch breadcrumbs.

It will enrich the breadcrumbs with request_body_size and response_body_size fields.

This works as follows:

For request body, we can access the body relatively easily. We handle different body types reasonably well.
For responses, we take the value of the Content-Length header, if available. No further processing needed!
- For xhr, we fall back to getting the response size from the actual response.
- For fetch, we cannot easily provide a fallback - see below.

For replay, we pass these through as requestBodySize and responseBodySize - we use camelCase there 😝

For regular breadcrumbs, this looks as follow in the current UI:

I refined types a bit to make this easier, we had a few places where we were still using incomplete types (and for replay, duplicating the types sometimes). This should now be reasonably type safe.

Caveats / notes

For fetch we do not get the response body size if the Content-Length header is not set. The reason for this is that this is async in fetch - we'd need to do:

const cloned = await response.clone(); // can only access text once, so need to clone the whole response
const body = await cloned.blob();
return getBodySize(body);

Closes #7373

mydea · 2023-03-10T11:10:51Z

packages/browser/src/integrations/breadcrumbs.ts

-function _xhrBreadcrumb(handlerData: HandlerData & { xhr: SentryWrappedXMLHttpRequest }): void {
-  if (handlerData.endTimestamp) {
-    // We only capture complete, non-sentry requests
-    if (handlerData.xhr.__sentry_own_request__) {


This is actually checked in the "original" breadcrumb integration in browser already, so no need to check this here again IMHO:
https://github.com/getsentry/sentry-javascript/blob/develop/packages/browser/src/integrations/breadcrumbs.ts#L222

Actually the same is also true for the endTimestamp/sentry_xhr fields, but there it makes more sense to guard them defensively again, I'd say.

mydea · 2023-03-10T11:12:22Z

packages/replay/src/coreHandlers/handleXhr.ts


-  if (!handlerData.endTimestamp) {
+  if (!startTimestamp || !endTimestamp || !xhr.__sentry_xhr__) {


Not 100% sure, but I don't think the "moving timestamps around & checking differnent places" stuff is really needed here (anymore), this should always be set, and IMHO if one of these fails we just bail out.

Makes sense, let's try it this way.

mydea · 2023-03-10T11:14:48Z

packages/utils/src/instrument.ts

-      const onreadystatechangeHandler = function (): void {
-        if (xhr.readyState === 4) {
+      const onreadystatechangeHandler: () => void = () => {
+        // For whatever reason, this is not the same instance here as from the outer method


This is extremely weird and I wasted a lot of time figuring out why this is happening, to no avail.

basically, it seems that when we just assign to xhrInfo from the outer scope, this is not the same as this.sentry_xhr, even though the this remains the same, and I don't see any other place where we'd set this 😬

Ooof no idea.. but given that integration tests are passing, I'd say let's just go with it...

github-actions · 2023-03-10T11:20:05Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	20.48 KB (+0.37% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	63.49 KB (+0.31% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	19.05 KB (+0.34% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)	56.39 KB (+0.32% 🔺)
@sentry/browser - Webpack (gzipped + minified)	20.7 KB (+0.26% 🔺)
@sentry/browser - Webpack (minified)	67.61 KB (+0.27% 🔺)
@sentry/react - Webpack (gzipped + minified)	20.73 KB (+0.25% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)	52.19 KB (+0.14% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	33.76 KB (+0.31% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	26.08 KB (+0.27% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	43.87 KB (+0.98% 🔺)
@sentry/replay - Webpack (gzipped + minified)	37.95 KB (+1.24% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	62.06 KB (+0.77% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	55.12 KB (+0.85% 🔺)

mydea · 2023-03-10T12:27:37Z

I will investigate if we can make this opt-in with less bundle size impact, let's see...

mydea · 2023-03-10T13:44:55Z

So I refactored this for a smaller bundle size impact, into a new optional integration. I updated the OP accordingly, IMHO this is also something we can then later reuse for adding e.g. the whole request/response body, if we ever need/want this.

scripts/node-unit-tests.ts

Lms24

LGTM! Had some minor comments but I like the refactor of the types and the idea with the optional integration. I think it's justified to tell replay users to use this integration if they want to monitor their request bodies (which I know we're not doing yet but eventually).

Lms24 · 2023-03-13T11:05:58Z

packages/types/src/client.ts

+  /**
+   * Register a callback before a breadcrumb is added.
+   */
+  on?(hook: 'beforeBreadcrumb', callback: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => void): void;


l: Hmm we already have a top-level callback called beforeBreadcrumb. my overall logaf is low here but do you think we should rename the hook to something like beforeAddBreadcrumb or something similar?

I'd just like to avoid confusion around the callback and the hook if that makes sense. Although, if we're moving into a hooks world, we might eventually even drop the top level callbacks or refactor them to use the hooks internally. So IMO we can also leave it as is

Yeah, I had the same thought. I guess beforeAddBreadcrumb makes sense 👍

packages/core/src/hub.ts

Lms24 · 2023-03-13T11:46:59Z

packages/integration-tests/suites/integrations/ExtendedNetworkBreadcrumbs/fetch/get/test.ts

+import { sentryTest } from '../../../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest } from '../../../../../utils/helpers';
+
+sentryTest('adds a breadcrumb for basic fetch GET request', async ({ getLocalTestPath, page }) => {


l: So here we're basically testing that the response_body_size is set based on the body size rather than the non-existing content-length header, right? Then let's reflect this in the test title.

I know we currently don't have integration tests for the normal breadcrumbs integration. I think we actually might want to add some for it because having a test with what the title suggests is still valuable IMO. Doesn't have to happen in this PR of course!

makes sense! 👍

Lms24 · 2023-03-13T11:49:33Z

packages/integration-tests/suites/integrations/ExtendedNetworkBreadcrumbs/xhr/get/test.ts

+import { sentryTest } from '../../../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest } from '../../../../../utils/helpers';
+
+sentryTest('adds a breadcrumb for basic XHR GET request', async ({ getLocalTestPath, page }) => {


l: Same thing here as for the equivalent fetch test

Lms24 · 2023-03-13T11:56:05Z

packages/replay/src/coreHandlers/handleXhr.ts


-  if (!handlerData.endTimestamp) {
+  if (!startTimestamp || !endTimestamp || !xhr.__sentry_xhr__) {


Makes sense, let's try it this way.

Lms24 · 2023-03-13T11:59:20Z

packages/utils/src/instrument.ts

-      const onreadystatechangeHandler = function (): void {
-        if (xhr.readyState === 4) {
+      const onreadystatechangeHandler: () => void = () => {
+        // For whatever reason, this is not the same instance here as from the outer method


Ooof no idea.. but given that integration tests are passing, I'd say let's just go with it...

billyvg

Is there a way we could automatically opt in Replay users to this integration? I think this is a pretty big barrier for what we should consider "basic" functionality for Replay.

mydea · 2023-03-13T15:19:05Z

LGTM! Had some minor comments but I like the refactor of the types and the idea with the optional integration. I think it's justified to tell replay users to use this integration if they want to monitor their request bodies (which I know we're not doing yet but eventually).

Yes, I also think it is a good separation of concerns to say "If you want any extended network monitoring, just use this integration (which can have settings in the future)", and keep the default experience as slim as possible :)

mydea · 2023-03-15T11:06:10Z

I updated this to be part of replay, it will be automatically applied there.

billyvg · 2023-03-15T14:37:35Z

packages/integrations/test/tsconfig.json

@@ -4,5 +4,5 @@
 {
  "extends": "../tsconfig.test.json",

-  "include": ["./**/*"]
+  "include": ["./**/*", "../../replay/test/unit/coreHandlers/extendednetworkbreadcrumbs.test.ts"]


is this case sensitive?

PR feedback rename tests & add breadcrumb tests browser integraiton tests move functionality to replay text encoder ensure we visit url avoid page goto? un-flake XHR/fetch tests revert node unit test changes?? improve replay test try make less brittle? skip xhr tests in non-chromium fix test

mydea requested review from billyvg, Lms24 and AbhiPrasad March 10, 2023 11:09

mydea self-assigned this Mar 10, 2023

mydea commented Mar 10, 2023

View reviewed changes

mydea force-pushed the fn/breadcrumbs-fetch-size branch from 37124b1 to afa7415 Compare March 10, 2023 13:42

mydea changed the title ~~feat(util): Add request_body_size & response_body_size to fetch/xhr~~ feat(integrations): Add request_body_size & response_body_size to fetch/xhr Mar 10, 2023

mydea changed the title ~~feat(integrations): Add request_body_size & response_body_size to fetch/xhr~~ feat(integrations): Add new optional ExtendedNetworkBreadcrumbs integration Mar 10, 2023

mydea commented Mar 10, 2023

View reviewed changes

scripts/node-unit-tests.ts Outdated Show resolved Hide resolved

Lms24 approved these changes Mar 13, 2023

View reviewed changes

billyvg reviewed Mar 13, 2023

View reviewed changes

mydea force-pushed the fn/breadcrumbs-fetch-size branch from efb115c to 8d07e33 Compare March 13, 2023 15:11

mydea force-pushed the fn/breadcrumbs-fetch-size branch 2 times, most recently from db663d1 to 0916dc3 Compare March 15, 2023 11:03

mydea changed the title ~~feat(integrations): Add new optional ExtendedNetworkBreadcrumbs integration~~ feat(replay): Capture request/response body size for xhr/fetch breadcrumbs Mar 15, 2023

billyvg reviewed Mar 15, 2023

View reviewed changes

mydea force-pushed the fn/breadcrumbs-fetch-size branch 4 times, most recently from 913064d to c63f22e Compare March 16, 2023 12:38

mydea force-pushed the fn/breadcrumbs-fetch-size branch from c63f22e to 9db62c7 Compare March 16, 2023 13:25

mydea merged commit 20b46a8 into develop Mar 16, 2023

mydea deleted the fn/breadcrumbs-fetch-size branch March 16, 2023 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(replay): Capture request/response body size for xhr/fetch breadcrumbs #7407

feat(replay): Capture request/response body size for xhr/fetch breadcrumbs #7407

mydea commented Mar 10, 2023 •

edited

Loading

mydea Mar 10, 2023

mydea Mar 10, 2023

Lms24 Mar 13, 2023

mydea Mar 10, 2023

Lms24 Mar 13, 2023

github-actions bot commented Mar 10, 2023 •

edited

Loading

mydea commented Mar 10, 2023

mydea commented Mar 10, 2023

Lms24 left a comment

Lms24 Mar 13, 2023

mydea Mar 14, 2023

Lms24 Mar 13, 2023

mydea Mar 13, 2023

Lms24 Mar 13, 2023

Lms24 Mar 13, 2023

Lms24 Mar 13, 2023

billyvg left a comment

mydea commented Mar 13, 2023

mydea commented Mar 15, 2023

billyvg Mar 15, 2023


		if (!handlerData.endTimestamp) {
		if (!startTimestamp \|\| !endTimestamp \|\| !xhr.__sentry_xhr__) {

feat(replay): Capture request/response body size for xhr/fetch breadcrumbs #7407

feat(replay): Capture request/response body size for xhr/fetch breadcrumbs #7407

Conversation

mydea commented Mar 10, 2023 • edited Loading

Caveats / notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 10, 2023 • edited Loading

size-limit report 📦

mydea commented Mar 10, 2023

mydea commented Mar 10, 2023

Lms24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billyvg left a comment

Choose a reason for hiding this comment

mydea commented Mar 13, 2023

mydea commented Mar 15, 2023

Choose a reason for hiding this comment

mydea commented Mar 10, 2023 •

edited

Loading

github-actions bot commented Mar 10, 2023 •

edited

Loading